header_rewrite: reject bad run-plugin at config load - #13493
Conversation
There was a problem hiding this comment.
🟢 Ready to approve
The changes prevent a confirmed crash path by rejecting invalid run-plugin rules at config load time, add defensive runtime behavior, and include a targeted gold test to validate both startup and reload scenarios.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR hardens the header_rewrite plugin’s run-plugin operator so that a target plugin load failure is rejected during config parsing (startup/reload) instead of leaving a null instance that can later abort the process on first request, and adds an AuTest gold test to cover both startup-failure and reload-rejection behavior.
Changes:
- Add operator validity checking (
Operator::is_valid()) and use it to reject invalidrun-pluginoperators during ruleset construction. - Convert
RuleSet::add_operator(and nestedifhandling) to usestd::unique_ptrownership to avoid manual deletes and make failure paths safer. - Add an end-to-end gold test that verifies bad
run-pluginconfigs fail cleanly at startup and are rejected on reload while keeping the prior running config.
File summaries
| File | Description |
|---|---|
| tests/gold_tests/pluginTest/header_rewrite/header_rewrite_bad_run_plugin.test.py | New gold test covering startup rejection and reload rejection for a bad run-plugin target. |
| plugins/header_rewrite/ruleset.h | Update RuleSet::add_operator overload to take std::unique_ptr<Operator> for RAII ownership. |
| plugins/header_rewrite/ruleset.cc | Build operators via std::unique_ptr, reject invalid operators at parse time, and transfer ownership safely into operator chains. |
| plugins/header_rewrite/operator.h | Introduce Operator::is_valid() hook for post-initialize validation. |
| plugins/header_rewrite/operators.h | Implement OperatorRunPlugin::is_valid() to reflect whether the target plugin loaded successfully. |
| plugins/header_rewrite/operators.cc | Improve run-plugin error reporting with config location, remove request-time assert in favor of defensive exec, and simplify argv handling via std::vector. |
| plugins/header_rewrite/header_rewrite.cc | Use std::unique_ptr for nested if stack and transfer ownership into rule/operator chains without manual deletes. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
3ea1d59 to
6b213a2
Compare
There was a problem hiding this comment.
🟢 Ready to approve
The changes correctly prevent null run-plugin instances from reaching request-time execution, improve safety and ownership, and add targeted regression coverage.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
6b213a2 to
7290191
Compare
There was a problem hiding this comment.
🟢 Ready to approve
It fixes a concrete crash path by rejecting invalid run-plugin rules at load time and includes a gold test that exercises both startup and reload scenarios.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
|
The core change looks right. Converting Replacing the One thing to flag for the 10.2.0 backport. This PR is tracked at The good news is that this looks cheap to resolve, because only the autest extension is missing — every runtime dependency it needs is already on 10.2.x:
So the extension file should drop onto 10.2.x and work. The wrinkle is that the commit which introduced it, #13075 ( Happy to do that as a small prerequisite pick if you and the RM agree that is the right shape. |
|
Correction to my note above, and it changes the recommendation. I said the autest extension was the only missing piece and suggested partial-picking just
So 10.2.x has the config-reload token-model feature but not its test-harness adoption or the Disregard the partial-pick suggestion; a normal |
A run-plugin whose target plugin failed to load left a null instance that tripped a release assert and aborted the server on the first request. Propagate the load failure as an exception so the rule is rejected at config load time (a reload simply keeps the running config), and guard exec() so a stray bad rule can never abort the process.
7290191 to
9eb4f82
Compare
This takes over #13425 from Leif Hedstrom (@zwoop), who authored the original change and the first commit in this PR.
A run-plugin whose target plugin failed to load left a null instance
that tripped a release assert and aborted the server on the first
request. Propagate the load failure as an exception so the rule is
rejected at config load time (a reload simply keeps the running
config), and guard exec() so a stray bad rule can never abort the
process.